[BugFix]: Fix local mypy issues#34739
Conversation
There was a problem hiding this comment.
Code Review
This pull request addresses several mypy issues found during local development. The changes primarily involve adding type: ignore comments to suppress type errors related to Pydantic dataclasses and dataclasses.replace, which is a reasonable approach for these known mypy limitations. A platform-specific check for os.sched_setaffinity has been correctly added to prevent errors on non-Linux systems. However, there's a potential issue in vllm/sampling_params.py where import json_mod is used. This seems to assume a new file that isn't part of the PR. I've suggested using a standard alias import json as json_mod to resolve the name clash cleanly.
4213f46 to
e9fe13b
Compare
Thanks @njhill for making me aware of it, I didn't see it when I checked before I started my PR. Yes, there is duplicate for |
e9fe13b to
f1a34f0
Compare
|
Thanks @hmellor for pointing out an existing implementation of |
|
Ok, I think I see. You lose the validation otherwise. |
|
@hmellor I have updated to use the pydantic compatible |
|
The main reason that And yes it allows us to continue using pydantic and all the validation that comes with it |
Review comment: - vllm-project#34739 (comment) Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
|
This pull request has merge conflicts that must be resolved before it can be |
MyPy issues handled as follows: - Pydantic dataclasses do not satisfy mypy's type system with replace(), even though they are fully compatible at runtime. Therefore, ignore - Skipped import issue StructuredOutputsParams in vllm/entrypoints/openai/responses/protocol.py. Therefore, ignore. - sched_setaffinity is Linux only OS command and therefore added check in llm/distributed/kv_transfer/kv_connector/v1/nixl_connector.py for it. Additionally: - Renamed json import because property called json in class also. This is for hygiene reasons. Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
Replace function from vllm/vllm/config/utils.py Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
Review comment: - vllm-project#34739 (comment) Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
693225f to
eebe7e1
Compare
hmellor
left a comment
There was a problem hiding this comment.
LGTM, thanks for helping reduce the mypy based weirdness in vLLM!
Thanks @hmellor for the reviews and feedback. |
Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com> Co-authored-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com> Co-authored-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com> Co-authored-by: Harry Mellor <19981378+hmellor@users.noreply.github.com> Signed-off-by: Andrii Skliar <askliar@nvidia.com>
Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com> Co-authored-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Purpose
When you run
pre-commitin local dev environment, you get the following mypy errors:This PR fixes the issues as shown below.
MyPy issues handled as follows:
replace(), even though they are fully compatible at runtime. Therefore, ignoreStructuredOutputsParamshas withjsonprioperty invllm/entrypoints/openai/responses/protocol.py. Therefore, ignore as enabling the imports raises lot of other issues which are unrelated.sched_setaffinity()is a Linux only OS command and therefore added a check inllm/distributed/kv_transfer/kv_connector/v1/nixl_connector.pyfor it.Additionally:
jsonimport because property calledjsonin class also invllm/sampling_params.py. This is for hygiene reasons.Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.